Initial feedback from skimming:

• I like the direction!
• In order to make it easy to review this, I'd want the change to be split into smaller commits. I'd suggest by isolating all the state (fields) into TransmitBuf without doing anything else in the first commit, then add the trait(s) in a separate commit and start moving over methods/adding abstractions.
• The BufMut + BufOffset bounds like repetitive, I guess BufOffset could have BufMut as a supertrait probably?
• (Nit: let's start moving towards using impl Trait instead of T: Trait in argument position.)

I've attempted to split this up, hopefully the commits are more or less what you had in mind. Still makes the same change essentially.

Thank you for working on this! I think this should be considered to close #2121

Thanks for your patience--I have ambitions of trying to take a closer look at this next weekend.

Yes, I think this is a good direction! Some thoughts so far...

Naming

First a minor thing: Perhaps this should be called TransmitBuilder rather than TransmitBuf? That way it matches PacketBuilder, and I feel that more accurately represents that that is tracking more complex state than just a byte sequence.

I don't have very strong feelings about the naming, this was really just a working name I thought would change but then just stuck as it did end up making sense. I think both work, but indeed TransmitBuilder would match PacketBuilder (which retains it's name in #2195). The current naming does match the fact it implements BufMut directly. I guess the variable name would also have to change if this was renamed, probably to transmit or so.

impl BufMut

With regard to this:

unsafe impl BufMut for TransmitBuf<'_>

I'm not sure that I like that. It adds an unsafe statement, as well as a handful of LOC, for the sake of being able to write buf instead of buf.buf in perhaps a dozen places. Plus, it's a slightly surprising thing to keep in mind.

What would you think of:

impl TransmitBuf<'_> {
    /// Returns a buffer into which the datagram can be written
    pub(super) fn datagram_mut(&mut self) -> bytes::buf::Limit<&mut Vec<u8>> {
        self.buf.limit(self.buf_capacity)
    }
}

This is very similar to what I did in #2195 which has a PacketBuilder::frame_space_mut method which I do rather like how it reads. And it gives protection against writing too many bytes into the buffer. Doing this would indeed alight well with renaming TransmitBuf to TransmitBuilder.

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

trait BufLen

You're introducing this new trait for the sake of being able to pass around &mut (impl BufMut + BufLen) instead of &mut TransmitBuf. I understand that your motivation for this is to abstract things, but in this circumstance I think that directly passing around &mut TransmitBuf or &mut Vec<u8> to various functions would be a net-win for it being simple to think about.

I did this trait for two reasons:

• A number of APIs expect this "I get large buffer with a max write offset (and sometimes need a start offset)" notion, which is what I'm trying to move away from. But to keep the scope of this PR smaller I can't change all of that at once, that would get out of hand. So this BufLen is a stopgap measure.
• I assumed that the functions which took traits rather than the &Vec<u8> wanted to keep doing this. There is some logic to this, because a number of these functions really don't have any business being able to change the state of the datagrams. They only need to write some stuff.

So I think if you receive an explicit &TransmitBuf (or whatever it is called) you need to have some business with datagrams. Only writing some frames is not that.

I think it might be possible to remove BufLen entirely and instead switch to BufMut::remaining_mut for these functions. But:

• I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.
• It would switch around a lot more logic. Using this BufLen keeps the changes to places where this stop-gap measure is needed much simpler and easier to review for correctness.

After #2195 there is only one BufLen remaining. I think it's very reasonable to also remove that and I'd be happy to look at doing that in another PR. Though I'd probably wait to figure out a nice way here until that has all been merged so I know what the final shape of things is and have more of a feel for what everyone agrees on for abstraction style.

Getter methods

I agree with djc here--I'm pretty hesitant about having these getter methods on TransmitBuf: segment_size, num_datagrams, max_datagrams, datagram_start_offset, and datagram_max_offset.

These variables aren't really getting abstracted away if the users of TransmitBuf still have to know which of them exist and care about their specific values. That leaves the only remaining motivation of using getters like this as making sure we don't accidentally mutate them wrongly from outside the module. But it's pretty easy to check where we're writing to these variables and I don't think making these variables read-only from outside this module really gets at the bottleneck of us being able to make the code correct.

This last sentence I am really not that sure about. Abstractions the compiler provides are great at ensuring correctness, having to manually check on each PR if something is being mutated that shouldn't be is very fallible. Having explicit contracts like this will make it clear if that is being changed.

And it has to be weighed against the cons of increased LOC, and us remembering which ones of these functions are simply reading a field versus which ones are doing something more transformative.

I was experimenting with an alternative way of trying to abstract some of these things, and I noticed that the num_datagrams value is used in similar ways in a few places repeatedly. So I tried to see if I could factor some of them away into more transformative methods on TransmitBuf until the num_datagrams value no longer needed to be accessed outside of TransmitBuf at all and I came up with this:

Yes, I agree there may be more scope here. And I was unsure whether to go further or not. For the sake of being more incremental with changes I stopped here for this PR though.

For example I think the bytes_to_send is probably a good one to add. But others I expect to be easier to remove in the future entirely. E.g. datagram_max_offset is very tempting to remove after #2195 where it is only 3 uses remain.

So, from what I understand maybe next steps for this PR could be:

• Rename TransmitBuf to TransmitBuilder
• Add TranmitBuiilder::datagram_mut function
• See which helpers might easily disappear with this, e.g. .len(), .is_empty() etc.

If you think that's helpful I'll give those changes a go.

@gretchenfrage I've pushed some new commits. Mainly this finally introduces a buffer abstraction that I've long wanted but resisted creating: the BufSlice (I'm not attached to this name, it could be better). To get access to the datagram being written into you need TransmitBuilder::datagram_mut() (or TransmitBuilder::datagram(), but in return you get a nice access to the buffer.

This means the PacketBuilder now needs to keep track of sizes relative to the datagram buffer, a change I've also long wanted. In turn this means some more of the questionable methods on TransmitBuilder can be removed, which is probably a good gain.

The main reason I was trying to avoid these changes so far was to try and keep the changes smaller. The abstractions are a fair number of lines of code again, but have a straight forward interface.

FWIW, TransmitBuilder::datagram_mut() is similar to the approach I already took in #2195 where I used PacketBuilder::frame_space_mut() to access the buffer. Give that model also works well there it gives me confidence this is a good approach.

It does leave a manual BufMut impl around, which I agree is unfortunate due to the unsafe. Though as before it mostly forwards calls to the the underlying buffer implementation. So I don't think this is so bad to have, and Quinn does not seem to have a history of avoiding unsafe.

Also note that BufSlice can probably completely remove the need for the BufLen trait from this PR. I'll do that once the BufSlice/DatagramBuffer idea has been accepted and possibly reshaped to reduce churn.

I'm still looking at this, but for what it's worth:

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

I don't believe that's true. Maybe I misunderstood? But checking out f2ecc76 and applying this diff seems to work just fine:

diff --git a/quinn-proto/src/connection/transmit_buf.rs b/quinn-proto/src/connection/transmit_buf.rs
index 31fe7e00..b429073d 100644
--- a/quinn-proto/src/connection/transmit_buf.rs
+++ b/quinn-proto/src/connection/transmit_buf.rs
@@ -141,7 +141,7 @@ impl<'a> TransmitBuf<'a> {
     }
 
     /// Returns a buffer into which the current datagram can be written
-    pub(super) fn datagram_mut(&mut self) -> bytes::buf::Limit<&mut Vec<u8>> {
+    pub(super) fn datagram_mut<'s>(&'s mut self) -> impl BufMut + super::BufLen + 's {
         self.buf.limit(self.buf_capacity)
     }

I'm not really statisfied from the BufMut::remaining_mut docs that this can be reliably used for this. I think the implementations we use, and certainly Limit, do. But it still leaves me a bit worried.

Can you expand on why not? From looking at it, it actually seems like it might be fine.

BufMut::remaining_mut:

Returns the number of bytes that can be written from the current position until the end of the buffer is reached.

This value is greater than or equal to the length of the slice returned by chunk_mut().

Implementer notes

Implementations of remaining_mut should ensure that the return value does not change unless a call is made to advance_mut or any other function that is documented to change the BufMut’s current position.

Note

remaining_mut may return value smaller than actual available space.

Notable source code bits exemplifying the expected usage of this:

• BufMut::put_slice panics if remaining_mut() < slice.len()
• <Vec<u8> as BufMut>::remaining_mut returns isize::MAX - vec.len()

The comment "This value is greater than or equal to the length of the slice returned by chunk_mut()" clarifies that it's not merely meant as the remaining length of the next chunk, and the put_slice implementation establishes that it's expected to indicate how many more bytes can be written. The Vec implementation seems to further reinforce this understanding. Given this, the comment of "remaining_mut may return value smaller than actual available space" seems to be a pretty low-information note that it can be lower than, say, the extent of the underlying heap allocation or equivalent.

If remaining_mut can be used instead of introducing a new BufLen trait, that sounds quite preferable.

I'm still looking at this, but for what it's worth:

(I'd like to return return a generic &mut impl BufMut there, but that doesn't currently work without use-lifetime annotations which are too recent a rust feature. Also that doesn't let me implement BufLen.)

I don't believe that's true. Maybe I misunderstood? But checking out f2ecc76 and applying this diff seems to work just fine:

[...] fn datagram_mut<'s>(&'s mut self) -> impl BufMut + BufLen + 's

Oh, interesting. Thanks for pointing this out.

Though in 4b0dcbf I now change that return type to DatagramBuffer. Which, while I failed to remove that comment, I do think can be a rather nice type to return rather than the trait. Because it clearly describes to any user what it is.

Feedback from skimming the current state:

• It looks mostly pretty good
• I think the trait hierarchy looks okay
• I don't like the trivial getters on TransmitBuilder, IMO we can just make the fields pub(super) instead
• The start_with_datagram_size() methods and related naming seems a bit unwieldy

I think it's definitely worth getting the commit history in shape here.

* I don't like the trivial getters on `TransmitBuilder`, IMO we can just make the fields `pub(super)` instead

I really would like to keep the access to fields read-only, and rust only allows this by using getter methods. When unfamiliar with the code, having everything access everything and mutate from various places make it rather hard to understand what is going on at times. It also results in logic about fields of structs being encoded in surprising places because everywhere can access it. I understand that originally this was convenient when first writing Quinn, but also think that it is fine to be more restrictive with the abstractions now.

* The `start_with_datagram_size()` methods and related naming seems a bit unwieldy

Yes, I was exploring this a bit more to see what can be done. This is basically an artifact of how the code was structured before. Having this code abstracted in one place makes it more obvious that this is a bit of a soar point. I think it would totally make sense to further refactor this. E.g. I can easily imagine inverting the api calls from "start new datagram" to
"finish datagram", it would probably remove the need to keep track of variables like num_datagrams as well. But I do think this comes up because inserting this abstraction makes is more easy to recognise this. I would prefer to not to add more to the current PR, I think it already is an improvement on what was there before.

I really would like to keep the access to fields read-only, and rust only allows this by using getter methods. When unfamiliar with the code, having everything access everything and mutate from various places make it rather hard to understand what is going on at times. It also results in logic about fields of structs being encoded in surprising places because everywhere can access it.

I understand this, but I think we can rely on callers in the limited visibility area to respect this if you just document it. Getters are just not worth it in this case.

Hi, just letting you know I'm not forgetting about this PR. Am still waiting for a round of feedback from @gretchenfrage before doing followup changes.

it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

Just a quick drive-by note -- it's not (just) that this is the style, but concretely it makes reviewing larger PRs much easier, since each commit can be reviewed in isolation.

it would be helpful for you to craft the PR commit history in a way that incorporates changes to commits into those commits, rather than by changing them with additional commits added to the end, because doing so meshes better with the git usage style of this project.

Just a quick drive-by note -- it's not (just) that this is the style, but concretely it makes reviewing larger PRs much easier, since each commit can be reviewed in isolation.

Well, if a project utilizes a "never rebase" workflow, that can make it easier to see what has changed about the PR since last review

Which is great for the brief period where you can exactly remember what happened last review 😁

(Still massaging commits to take feedback into account, these newly pushed commits are not yet ready for a round of review. Will re-request review once ready again.)